Skip to content

Fix build CI failure on MacOS#1685

Merged
junqiu-lei merged 2 commits intoopensearch-project:mainfrom
junqiu-lei:fix-macos-ci
May 3, 2024
Merged

Fix build CI failure on MacOS#1685
junqiu-lei merged 2 commits intoopensearch-project:mainfrom
junqiu-lei:fix-macos-ci

Conversation

@junqiu-lei
Copy link
Member

@junqiu-lei junqiu-lei commented May 2, 2024

Description

Fix build CI failure for MacOS. The github CI runner for macos-latest changed to use ARM platform recently, updating to use macos-12 with x86_64 architecture to pass the CI.

Issues Resolved

#1660

Check List

  • Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@junqiu-lei junqiu-lei force-pushed the fix-macos-ci branch 7 times, most recently from 9cd8cc7 to fe91a0f Compare May 3, 2024 00:21
@junqiu-lei junqiu-lei changed the title Try to fix build CI failure for MacOS Fix build CI failure on MacOS May 3, 2024
@junqiu-lei junqiu-lei added the Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label May 3, 2024
@junqiu-lei
Copy link
Member Author

junqiu-lei commented May 3, 2024

Currently k-NN native libraries can completely make build in macos-latest CI runner env, the new error occurred in integ test:

»  java.lang.UnsatisfiedLinkError: no opensearchknn_common in java.library.path: [/Users/runner/work/k-NN/k-NN/jni/release]
»  	at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2678)
»  	at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:830)
»  	at java.base/java.lang.System.loadLibrary(System.java:1890)
»  	at org.opensearch.knn.jni.JNICommons.lambda$static$0(JNICommons.java:26)
»  	at java.base/java.security.AccessController.doPrivileged(Native Method)
»  	at org.opensearch.knn.jni.JNICommons.<clinit>(JNICommons.java:25)
»  	at org.opensearch.knn.index.codec.util.KNNCodecUtil.getFloats(KNNCodecUtil.java:89)
»  	at 

sed -i -e 's/__aarch64__/__undefine_aarch64__/g' external/faiss/faiss/utils/distances_simd.cpp
sed -i -e 's/pragma message WARN/pragma message /g' external/nmslib/similarity_search/src/distcomp_scalar.cc
sed -i -e 's/-march=native/-mcpu=apple-m1/g' external/nmslib/similarity_search/CMakeLists.txt
sed -i -e 's/-mcpu=apple-a14/-mcpu=apple-m1/g' external/nmslib/python_bindings/setup.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we dont use python bindings so this should be unnecessary.

sed -i -e 's/pragma message WARN/pragma message /g' external/nmslib/similarity_search/src/distcomp_scalar.cc
sed -i -e 's/-march=native/-mcpu=apple-m1/g' external/nmslib/similarity_search/CMakeLists.txt
sed -i -e 's/-mcpu=apple-a14/-mcpu=apple-m1/g' external/nmslib/python_bindings/setup.py
if sysctl -n machdep.cpu.features | grep -i AVX2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont need this if runner is arm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as below

export CC=/opt/homebrew/opt/llvm/bin/clang
export CXX=/opt/homebrew/opt/llvm/bin/clang++
cd jni
sed -i -e 's/\/usr\/local\/opt\/libomp\//\/opt\/homebrew\/opt\/llvm\//g' cmake/init-faiss.cmake
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just add this check in cmake?

export CXX=/opt/homebrew/opt/llvm/bin/clang++
cd jni
sed -i -e 's/\/usr\/local\/opt\/libomp\//\/opt\/homebrew\/opt\/llvm\//g' cmake/init-faiss.cmake
sed -i -e 's/__aarch64__/__undefine_aarch64__/g' external/faiss/faiss/utils/distances_simd.cpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be possible to avoid. In faiss, I think they are building it without patches: facebookresearch/faiss#3411.

cd jni
sed -i -e 's/\/usr\/local\/opt\/libomp\//\/opt\/homebrew\/opt\/llvm\//g' cmake/init-faiss.cmake
sed -i -e 's/__aarch64__/__undefine_aarch64__/g' external/faiss/faiss/utils/distances_simd.cpp
sed -i -e 's/pragma message WARN/pragma message /g' external/nmslib/similarity_search/src/distcomp_scalar.cc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to switch the WARN message?

sed -i -e 's/\/usr\/local\/opt\/libomp\//\/opt\/homebrew\/opt\/llvm\//g' cmake/init-faiss.cmake
sed -i -e 's/__aarch64__/__undefine_aarch64__/g' external/faiss/faiss/utils/distances_simd.cpp
sed -i -e 's/pragma message WARN/pragma message /g' external/nmslib/similarity_search/src/distcomp_scalar.cc
sed -i -e 's/-march=native/-mcpu=apple-m1/g' external/nmslib/similarity_search/CMakeLists.txt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that native should be fine. We shouldnt need this

@junqiu-lei
Copy link
Member Author

Thank @jmazanec15 for comments. For the arm setup, I followed steps from k-NN developer guide doc. But currently the it has another new error happened during k-NN run time in integ test when we use the macos-latest.

@junqiu-lei
Copy link
Member Author

junqiu-lei commented May 3, 2024

After we just changed the label to use macos-12(x86_64 architecture) which is used from most recent successful run, the CI on MacOS can be passed. https://github.com/opensearch-project/k-NN/actions/runs/8942266898/job/24564490315?pr=1685

@junqiu-lei junqiu-lei marked this pull request as ready for review May 3, 2024 17:48
brew reinstall gcc
export FC=/usr/local/Cellar/gcc/12.2.0/bin/gfortran
brew install llvm
brew install openblas
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this message in logs:

2024-05-03T00:54:26.3237060Z openblas is keg-only, which means it was not symlinked into /opt/homebrew,
2024-05-03T00:54:26.3237780Z because macOS provides BLAS in Accelerate.framework.

Can we skip this?

- name: Build native libraries
run: |
git submodule update --init --recursive
brew reinstall gcc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're using clang, I dont think we need gcc

@junqiu-lei junqiu-lei merged commit 73d5425 into opensearch-project:main May 3, 2024
@junqiu-lei junqiu-lei deleted the fix-macos-ci branch May 3, 2024 22:27
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 3, 2024
Signed-off-by: Junqiu Lei <[email protected]>
(cherry picked from commit 73d5425)
junqiu-lei pushed a commit that referenced this pull request May 6, 2024
jingqimao77-spec pushed a commit to jingqimao77-spec/k-NN that referenced this pull request Mar 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 2.x Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. skip-changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants